Skip to content

Conversation

@LucasG0
Copy link
Contributor

@LucasG0 LucasG0 commented Sep 24, 2025

Use classes within SDK to avoid duplication.
ConvertObjectType mutation now accepts use_default_value in order to specify when we want of a field to use the default value instead of potentially reusing the value of an existing matching input field. Corresponding sdk PR: opsmill/infrahub-sdk-python#558

Summary by CodeRabbit

  • New Features

    • Improved object-type conversion with direct field mappings, raw-value extraction, and explicit default-value handling; relationship conversions (single/multiple) more robust.
    • Converted objects now preserve additional optional attributes (e.g., favorite_color).
  • Refactor

    • Public conversion input types replaced with new SDK-provided wrappers; callers and integrations should adopt the new input structures.
  • Tests

    • Expanded tests to cover new input types, default-value behavior, and optional-attribute preservation.
  • Chores

    • SDK subproject reference bumped.

@LucasG0 LucasG0 requested a review from a team as a code owner September 24, 2025 13:20
@LucasG0 LucasG0 marked this pull request as draft September 24, 2025 13:20
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 24, 2025

Walkthrough

Pydantic-based input models (InputForDestField/InputDataForDestField) were removed and replaced with ConversionFieldInput and ConversionFieldValue from infrahub_sdk. Core conversion functions and signatures (object, repository, GraphQL mutation) were updated to use ConversionFieldInput mappings. Data building now reads from source_field or explicit data, handles BaseAttribute and RelationshipManager (ONE/MANY), and respects use_default_value; a helper extracts raw values from ConversionFieldValue. Tests were adapted and a favorite_color Text attribute with default "blue" was added. The python_sdk subproject reference was bumped.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly specifies the predominant change in this pull request: migrating all mapping definitions to use the SDK’s ConversionFieldInput type. It directly refers to the core refactoring in object conversion, repository conversion, GraphQL mutation, and tests. It is concise, avoids generic wording, and gives reviewers immediate clarity on the purpose of the update.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch lgu-obj-conv-default-value

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions bot added the group/backend Issue related to the backend (API Server, Git Agent) label Sep 24, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented Sep 24, 2025

CodSpeed Performance Report

Merging #7258 will not alter performance

Comparing lgu-obj-conv-default-value (55f404f) with develop (6928235)

Summary

✅ 10 untouched

@LucasG0 LucasG0 marked this pull request as ready for review September 24, 2025 14:17
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (11)
backend/infrahub/core/convert_object_type/repository_conversion.py (2)

4-8: Import ConversionFieldInput directly from the SDK

Avoid re-export coupling; import the public type from infrahub_sdk and keep core conversions from the local module.

Apply this diff:

-from infrahub.core.convert_object_type.object_conversion import (
-    ConversionFieldInput,
-    convert_object_type,
-    validate_conversion,
-)
+from infrahub_sdk.convert_object_type import ConversionFieldInput
+from infrahub.core.convert_object_type.object_conversion import convert_object_type, validate_conversion

28-33: Docstring should follow Google style and mention parameters/returns

The function meets behavior requirements but the docstring format violates project guidelines.

Apply this diff to upgrade the docstring:

-    """Delete the node and return the new created one. If creation fails, the node is not deleted, and raise an error.
-    An extra check is performed on input node peers relationships to make sure they are still valid."""
+    """Convert a repository node to a target schema.
+
+    Deletes the original repository node, creates a new one with the target schema, and reattaches validators.
+    Performs validation post-conversion and refreshes branch registries.
+
+    Args:
+        repository: The repository node to convert.
+        target_schema: Destination NodeSchema to convert to.
+        mapping: Field mapping instructions (source field, explicit data, or default).
+        branch: Branch in which to perform the conversion.
+        db: Database connection.
+        repository_post_creator: Finalizer to run after repository creation (e.g., repo init).
+
+    Returns:
+        The newly created repository node.
+
+    Raises:
+        ValueError: If conversion or validation fails.
+    """
backend/tests/functional/convert_object_type/test_convert_repositories.py (2)

12-13: Import SDK types directly for consistency with the public API

Tests should import ConversionFieldInput/ConversionFieldValue from infrahub_sdk to match production usage and PR intent.

Apply this diff:

-from infrahub.core.convert_object_type.object_conversion import ConversionFieldInput, ConversionFieldValue
+from infrahub_sdk.convert_object_type import ConversionFieldInput, ConversionFieldValue

211-218: Optional: build mapping via dict comprehension

Minor readability win; logic unchanged.

Example for each occurrence:

-        mapping = {}
-        for field_name, field_infos in conversion_response["FieldsMappingTypeConversion"]["mapping"].items():
-            if field_infos["source_field_name"] is not None:
-                mapping[field_name] = ConversionFieldInput(source_field=field_infos["source_field_name"])
-            else:
-                assert field_name == "ref"
+        mapping = {
+            name: ConversionFieldInput(source_field=info["source_field_name"])
+            for name, info in conversion_response["FieldsMappingTypeConversion"]["mapping"].items()
+            if info["source_field_name"] is not None
+        }

Keep the explicit special-case assignment (ref/default_branch) below as-is.

Also applies to: 357-364, 495-503

backend/infrahub/graphql/mutations/convert_object_type.py (2)

9-9: Import ConversionFieldInput from the SDK to avoid re-export dependency

Keep convert_and_validate_object_type from the local module; pull the type from the SDK.

Apply this diff:

-from infrahub.core.convert_object_type.object_conversion import ConversionFieldInput, convert_and_validate_object_type
+from infrahub_sdk.convert_object_type import ConversionFieldInput
+from infrahub.core.convert_object_type.object_conversion import convert_and_validate_object_type

57-59: Avoid duplicate node fetch

node_to_convert is already retrieved at Lines 43-45. The second query is redundant.

Apply this diff:

-        node_to_convert = await NodeManager.get_one(
-            id=str(data.node_id), db=graphql_context.db, branch=graphql_context.branch
-        )
+        # Reuse node_to_convert fetched above
backend/tests/unit/core/convert_object_type/test_convert_object_type.py (1)

12-15: Import SDK conversion types instead of re-exported symbols

Align unit tests with the public SDK API.

Apply this diff:

-from infrahub.core.convert_object_type.object_conversion import (
-    ConversionFieldInput,
-    ConversionFieldValue,
-    convert_and_validate_object_type,
-)
+from infrahub_sdk.convert_object_type import ConversionFieldInput, ConversionFieldValue
+from infrahub.core.convert_object_type.object_conversion import convert_and_validate_object_type
backend/infrahub/core/convert_object_type/object_conversion.py (4)

23-31: Add a docstring to _get_conversion_field_raw_value

Project guidelines require Google-style docstrings.

Apply this diff:

-def _get_conversion_field_raw_value(conv_field_value: ConversionFieldValue) -> Any:
+def _get_conversion_field_raw_value(conv_field_value: ConversionFieldValue) -> Any:
+    """Extract the raw payload from a ConversionFieldValue.
+
+    Prefers attribute_value, then peer_id, then peers_ids.
+
+    Args:
+        conv_field_value: The value wrapper containing either an attribute value or relationship id(s).
+
+    Returns:
+        The raw Python value to be passed to node creation.
+
+    Raises:
+        ValueError: If none of the supported fields are set.
+    """

42-46: Tighten return type and docstring format for build_data_new_node

  • Use dict[str, Any] per typing best practices.
  • Docstring should follow Google style with Args/Returns.

Apply this diff:

-async def build_data_new_node(db: InfrahubDatabase, mapping: dict[str, ConversionFieldInput], node: Node) -> dict:
-    """Value of a given field on the target kind to convert is either an input source attribute/relationship of the source node,
-    or a raw value."""
+async def build_data_new_node(
+    db: InfrahubDatabase, mapping: dict[str, ConversionFieldInput], node: Node
+) -> dict[str, Any]:
+    """Build creation payload for the new node.
+
+    For each destination field, either pulls from a source field on the original node, uses explicit data,
+    or relies on default values when requested.
+
+    Args:
+        db: Database connection.
+        mapping: Destination field mapping instructions.
+        node: Source node being converted.
+
+    Returns:
+        A dict payload consumable by create_node.
+    """

123-134: Add a docstring to convert_and_validate_object_type

Required by guidelines and helpful for future readers.

Apply this diff:

-async def convert_and_validate_object_type(
+async def convert_and_validate_object_type(
     node: Node,
     target_schema: NodeSchema,
     mapping: dict[str, ConversionFieldInput],
     branch: Branch,
     db: InfrahubDatabase,
 ) -> Node:
+    """Convert a node and validate peer constraints post-conversion.
+
+    Wraps conversion in a transaction, validates relationships, and refreshes branch registries.
+
+    Args:
+        node: The node to convert.
+        target_schema: Destination NodeSchema to convert to.
+        mapping: Field mapping instructions (source field, explicit data, or default).
+        branch: Branch in which to perform the conversion.
+        db: Database connection.
+
+    Returns:
+        The newly created node.
+    """

146-155: Docstring is present; consider Google-style expansion (optional)

Optional to expand with Args/Returns per guidelines for consistency with the module.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 959850b and 71c0996.

📒 Files selected for processing (8)
  • backend/infrahub/core/convert_object_type/object_conversion.py (7 hunks)
  • backend/infrahub/core/convert_object_type/repository_conversion.py (2 hunks)
  • backend/infrahub/graphql/mutations/convert_object_type.py (3 hunks)
  • backend/tests/conftest.py (1 hunks)
  • backend/tests/functional/convert_object_type/test_convert_object_type.py (4 hunks)
  • backend/tests/functional/convert_object_type/test_convert_repositories.py (4 hunks)
  • backend/tests/unit/core/convert_object_type/test_convert_object_type.py (9 hunks)
  • python_sdk (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
backend/tests/**/*

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Place backend tests in backend/tests/

Files:

  • backend/tests/conftest.py
  • backend/tests/functional/convert_object_type/test_convert_repositories.py
  • backend/tests/functional/convert_object_type/test_convert_object_type.py
  • backend/tests/unit/core/convert_object_type/test_convert_object_type.py
backend/**/*

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Run backend tests with pytest or via invoke tasks

Files:

  • backend/tests/conftest.py
  • backend/tests/functional/convert_object_type/test_convert_repositories.py
  • backend/tests/functional/convert_object_type/test_convert_object_type.py
  • backend/tests/unit/core/convert_object_type/test_convert_object_type.py
  • backend/infrahub/graphql/mutations/convert_object_type.py
  • backend/infrahub/core/convert_object_type/repository_conversion.py
  • backend/infrahub/core/convert_object_type/object_conversion.py
**/*.py

📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)

**/*.py: Use triple double quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include a brief one-line description at the top of each docstring
Add a detailed description section when additional context is needed
Document function/method parameters under an Args/Parameters section without typing information
Include a Returns section describing the return value
Include a Raises section listing possible exceptions
Provide an Examples section demonstrating usage when helpful

**/*.py: Use type hints for all function parameters and return values in Python code
Prefer asynchronous implementations when feasible (use Async whenever possible)
Define asynchronous functions with async def
Use await for asynchronous calls
Use Pydantic models instead of standard library dataclasses
All Python functions must include a docstring
Use Ruff and MyPy for linting and type checking

Use ruff and mypy to validate and lint Python files

**/*.py: Use type hints for all Python function parameters and return values
Use async/await whenever possible in Python code
Use async def for asynchronous functions
Use await for asynchronous calls within async functions
Use Pydantic models instead of dataclasses for data modeling
All Python functions must have docstrings
Docstrings must use triple quotes (""")
Follow Google-style docstring format
Docstrings should include, when applicable: brief description, detailed description, Args/Parameters (without types), Returns, Raises, and Examples
Validate and lint Python with ruff and mypy
Format Python code using the project’s formatter (invoke format)

Files:

  • backend/tests/conftest.py
  • backend/tests/functional/convert_object_type/test_convert_repositories.py
  • backend/tests/functional/convert_object_type/test_convert_object_type.py
  • backend/tests/unit/core/convert_object_type/test_convert_object_type.py
  • backend/infrahub/graphql/mutations/convert_object_type.py
  • backend/infrahub/core/convert_object_type/repository_conversion.py
  • backend/infrahub/core/convert_object_type/object_conversion.py
🧬 Code graph analysis (4)
backend/tests/functional/convert_object_type/test_convert_repositories.py (1)
backend/infrahub/core/convert_object_type/object_conversion.py (1)
  • convert_object_type (146-201)
backend/tests/functional/convert_object_type/test_convert_object_type.py (1)
backend/infrahub/core/convert_object_type/object_conversion.py (1)
  • convert_object_type (146-201)
backend/tests/unit/core/convert_object_type/test_convert_object_type.py (1)
backend/infrahub/core/convert_object_type/schema_mapping.py (1)
  • SchemaMappingValue (7-10)
backend/infrahub/graphql/mutations/convert_object_type.py (1)
backend/infrahub/core/convert_object_type/object_conversion.py (2)
  • convert_object_type (146-201)
  • convert_and_validate_object_type (123-143)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Cloudflare Pages
🔇 Additional comments (7)
backend/tests/conftest.py (1)

1256-1259: Good addition: defaultable test attribute

Adding favorite_color with a default aligns with the new use_default_value path and widens coverage.

backend/infrahub/core/convert_object_type/repository_conversion.py (1)

23-23: Signature update LGTM

Using dict[str, ConversionFieldInput] keeps the server aligned with the SDK-facing API.

backend/tests/functional/convert_object_type/test_convert_object_type.py (1)

62-66: Nice coverage for default override behavior

  • favorite_color mapping + assertion verify use_default_value precedence over auto-mapping and source value.
  • Mapping construction via SDK types is correct.

Also applies to: 127-134, 153-153

backend/infrahub/graphql/mutations/convert_object_type.py (1)

50-56: Field mapping parsing and auto-mapping look correct

  • Pydantic parsing of GenericScalar payload into ConversionFieldInput works.
  • Auto-mapping fills only missing fields.

Also applies to: 63-66

backend/tests/unit/core/convert_object_type/test_convert_object_type.py (1)

39-39: Tests updated correctly to new conversion API

  • Mapping count increase and favorite_color assertions reflect the schema change.
  • Mapping construction via SDK types is correct across scenarios.

Also applies to: 44-46, 110-118, 141-141, 182-182, 193-195, 260-263, 311-316

backend/infrahub/core/convert_object_type/object_conversion.py (2)

63-69: Verify relationship payload shapes from explicit data

When using conv_field_input.data for relationships, this code passes raw id(s) (str | list[str]) whereas source_field paths build {"id": ...} structures. Ensure create_node accepts both shapes for relationships.

If needed, I can adjust build_data_new_node to normalize explicit relationship data to {"id": ...} / [{"id": ...}] consistently, but that would require access to target schema cardinalities. Confirm acceptance of raw id(s) in create_node.


98-104: Pass‑through aware attribute detection is sound

Only source_field mappings are considered pass-through, excluding defaults/explicit data. This matches the intended branch behavior.

Copy link
Contributor

@ajtmccarty ajtmccarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you link the PR for the sdk change in the description of this one please?

@LucasG0 LucasG0 force-pushed the lgu-obj-conv-default-value branch from 71c0996 to 1efa4e6 Compare September 24, 2025 22:57
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
backend/infrahub/graphql/mutations/convert_object_type.py (1)

32-38: Expand docstring and remove duplicate fetch

  • Add a Google-style docstring (Args/Returns/Raises) to mutate in backend/infrahub/graphql/mutations/convert_object_type.py.
  • Remove the second await NodeManager.get_one(...) call (lines 57–59) since the node is already fetched at line 43.
🧹 Nitpick comments (4)
backend/infrahub/graphql/mutations/convert_object_type.py (2)

9-9: Import ConversionFieldInput from the SDK to avoid duplication

Switch to the SDK model per PR objective. Keep convert_and_validate_object_type import from core.

-from infrahub.core.convert_object_type.object_conversion import ConversionFieldInput, convert_and_validate_object_type
+from infrahub_sdk.convert_object_type import ConversionFieldInput
+from infrahub.core.convert_object_type.object_conversion import convert_and_validate_object_type

54-56: Add defensive type-checking for mapping entries

GraphQL GenericScalar can pass unexpected shapes. Guard against non-dict values to fail fast with a clear error.

-for field_name, input_for_dest_field_str in data.fields_mapping.items():
-    fields_mapping[field_name] = ConversionFieldInput(**input_for_dest_field_str)
+for field_name, input_for_dest_field in data.fields_mapping.items():
+    if not isinstance(input_for_dest_field, dict):
+        raise ValueError(
+            f"Expected mapping value for '{field_name}' to be a dict, got {type(input_for_dest_field)}"
+        )
+    fields_mapping[field_name] = ConversionFieldInput(**input_for_dest_field)
backend/tests/functional/convert_object_type/test_convert_repositories.py (1)

12-12: Use SDK types in tests for consistency

Align with SDK-based models everywhere.

-from infrahub.core.convert_object_type.object_conversion import ConversionFieldInput, ConversionFieldValue
+from infrahub_sdk.convert_object_type import ConversionFieldInput, ConversionFieldValue
backend/tests/unit/core/convert_object_type/test_convert_object_type.py (1)

12-15: Switch tests to use SDK models; keep core conversion function import

Avoid mixed sources for the same models across the codebase.

-from infrahub.core.convert_object_type.object_conversion import (
-    ConversionFieldInput,
-    ConversionFieldValue,
-    convert_and_validate_object_type,
-)
+from infrahub_sdk.convert_object_type import ConversionFieldInput, ConversionFieldValue
+from infrahub.core.convert_object_type.object_conversion import convert_and_validate_object_type
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 71c0996 and 1efa4e6.

📒 Files selected for processing (8)
  • backend/infrahub/core/convert_object_type/object_conversion.py (7 hunks)
  • backend/infrahub/core/convert_object_type/repository_conversion.py (2 hunks)
  • backend/infrahub/graphql/mutations/convert_object_type.py (3 hunks)
  • backend/tests/conftest.py (1 hunks)
  • backend/tests/functional/convert_object_type/test_convert_object_type.py (4 hunks)
  • backend/tests/functional/convert_object_type/test_convert_repositories.py (4 hunks)
  • backend/tests/unit/core/convert_object_type/test_convert_object_type.py (9 hunks)
  • python_sdk (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • python_sdk
  • backend/infrahub/core/convert_object_type/repository_conversion.py
  • backend/tests/conftest.py
🧰 Additional context used
📓 Path-based instructions (3)
backend/tests/**/*

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Place backend tests in backend/tests/

Files:

  • backend/tests/functional/convert_object_type/test_convert_object_type.py
  • backend/tests/functional/convert_object_type/test_convert_repositories.py
  • backend/tests/unit/core/convert_object_type/test_convert_object_type.py
backend/**/*

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Run backend tests with pytest or via invoke tasks

Files:

  • backend/tests/functional/convert_object_type/test_convert_object_type.py
  • backend/infrahub/graphql/mutations/convert_object_type.py
  • backend/tests/functional/convert_object_type/test_convert_repositories.py
  • backend/tests/unit/core/convert_object_type/test_convert_object_type.py
  • backend/infrahub/core/convert_object_type/object_conversion.py
**/*.py

📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)

**/*.py: Use triple double quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include a brief one-line description at the top of each docstring
Add a detailed description section when additional context is needed
Document function/method parameters under an Args/Parameters section without typing information
Include a Returns section describing the return value
Include a Raises section listing possible exceptions
Provide an Examples section demonstrating usage when helpful

**/*.py: Use type hints for all function parameters and return values in Python code
Prefer asynchronous implementations when feasible (use Async whenever possible)
Define asynchronous functions with async def
Use await for asynchronous calls
Use Pydantic models instead of standard library dataclasses
All Python functions must include a docstring
Use Ruff and MyPy for linting and type checking

Use ruff and mypy to validate and lint Python files

**/*.py: Use type hints for all Python function parameters and return values
Use async/await whenever possible in Python code
Use async def for asynchronous functions
Use await for asynchronous calls within async functions
Use Pydantic models instead of dataclasses for data modeling
All Python functions must have docstrings
Docstrings must use triple quotes (""")
Follow Google-style docstring format
Docstrings should include, when applicable: brief description, detailed description, Args/Parameters (without types), Returns, Raises, and Examples
Validate and lint Python with ruff and mypy
Format Python code using the project’s formatter (invoke format)

Files:

  • backend/tests/functional/convert_object_type/test_convert_object_type.py
  • backend/infrahub/graphql/mutations/convert_object_type.py
  • backend/tests/functional/convert_object_type/test_convert_repositories.py
  • backend/tests/unit/core/convert_object_type/test_convert_object_type.py
  • backend/infrahub/core/convert_object_type/object_conversion.py
🧬 Code graph analysis (4)
backend/tests/functional/convert_object_type/test_convert_object_type.py (1)
backend/infrahub/core/convert_object_type/object_conversion.py (1)
  • convert_object_type (146-201)
backend/infrahub/graphql/mutations/convert_object_type.py (1)
backend/infrahub/core/convert_object_type/object_conversion.py (2)
  • convert_object_type (146-201)
  • convert_and_validate_object_type (123-143)
backend/tests/functional/convert_object_type/test_convert_repositories.py (1)
backend/infrahub/core/convert_object_type/object_conversion.py (1)
  • convert_object_type (146-201)
backend/tests/unit/core/convert_object_type/test_convert_object_type.py (1)
backend/infrahub/core/convert_object_type/schema_mapping.py (1)
  • SchemaMappingValue (7-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: backend-benchmark
  • GitHub Check: E2E-testing-playwright
  • GitHub Check: validate-generated-documentation
  • GitHub Check: backend-tests-functional
  • GitHub Check: backend-tests-integration
  • GitHub Check: backend-tests-unit
🔇 Additional comments (11)
backend/tests/functional/convert_object_type/test_convert_object_type.py (4)

62-66: Expected mapping updated with favorite_color — LGTM


120-120: Seeding favorite_color on source node to validate default override — LGTM


127-134: Exercise of use_default_value=True and mixed data/source mappings — LGTM


6-6: Tests already standardized to SDK imports; no changes needed.

Likely an incorrect or invalid review comment.

backend/infrahub/graphql/mutations/convert_object_type.py (1)

61-66: Auto-mapping logic — LGTM

Completes missing fields where a source field exists without overriding explicit user inputs.

backend/tests/functional/convert_object_type/test_convert_repositories.py (3)

213-218: Mapping ref via attribute_value — LGTM

Clear example of supplying an explicit value for a field with no source mapping.


359-364: Mapping default_branch via attribute_value — LGTM


498-503: Repeat pattern for default_branch mapping — LGTM

backend/tests/unit/core/convert_object_type/test_convert_object_type.py (3)

39-39: Updated mapping expectations (favorite_color) — LGTM

Also applies to: 44-46


110-118: New ConversionFieldInput/Value usage in unit mapping — LGTM


141-141: Assertion on favorite_color default — LGTM

Assuming schema default is set; see earlier verification note.

@LucasG0 LucasG0 force-pushed the lgu-obj-conv-default-value branch from 1efa4e6 to f2f657a Compare September 25, 2025 09:39
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
backend/tests/unit/core/convert_object_type/test_convert_object_type.py (1)

226-228: Fix kind mismatch when refreshing jack_1

Using "TestmoPerson1" here is incorrect; the node was created with schema "TestudPerson1". This will cause a lookup failure.

Apply:

-            db=db, id=jack_1.id, kind="TestmoPerson1", prefetch_relationships=True, branch=default_branch
+            db=db, id=jack_1.id, kind="TestudPerson1", prefetch_relationships=True, branch=default_branch
🧹 Nitpick comments (8)
backend/tests/functional/convert_object_type/test_convert_object_type.py (1)

127-134: Nit: Use created car_3 in slowest_cars to reflect intent

Mapping slowest_cars to car_1 while also creating car_3 is confusing. Prefer using car_3 for clarity (mirrors the unit test).

Apply:

-            "slowest_cars": ConversionFieldInput(data=ConversionFieldValue(peers_ids=[car_1.id])),
+            "slowest_cars": ConversionFieldInput(data=ConversionFieldValue(peers_ids=[car_3.id])),
backend/tests/functional/convert_object_type/test_convert_repositories.py (1)

12-12: Import ConversionField from SDK for consistency*

Other tests import these types from infrahub_sdk. Importing from the SDK avoids tighter coupling to internal modules.

Apply:

-from infrahub.core.convert_object_type.object_conversion import ConversionFieldInput, ConversionFieldValue
+from infrahub_sdk.convert_object_type import ConversionFieldInput, ConversionFieldValue
backend/tests/unit/core/convert_object_type/test_convert_object_type.py (1)

12-14: Import ConversionField from SDK, keep core import only for convert function*

Reduces coupling and matches project-wide move to SDK types.

Apply:

-from infrahub.core.convert_object_type.object_conversion import (
-    ConversionFieldInput,
-    ConversionFieldValue,
-    convert_and_validate_object_type,
-)
+from infrahub_sdk.convert_object_type import ConversionFieldInput, ConversionFieldValue
+from infrahub.core.convert_object_type.object_conversion import convert_and_validate_object_type
backend/infrahub/core/convert_object_type/object_conversion.py (5)

33-40: Add docstring and precise return type

Comply with project docstring/type-hint guidelines.

Apply:

-async def get_out_rels_peers_ids(node: Node, db: InfrahubDatabase, at: Timestamp) -> list[str]:
+async def get_out_rels_peers_ids(node: Node, db: InfrahubDatabase, at: Timestamp) -> list[str]:
+    """Collect outbound peers' IDs for a node at a timestamp.
+
+    Args:
+        node: Node to inspect.
+        db: Database handle.
+        at: Point-in-time to query.
+
+    Returns:
+        List of peer IDs for all outbound relationships.
+    """

42-69: Tighten typing and docstring; clarify return shape

Return type should be dict[str, Any]. Expand docstring per guidelines.

Apply:

-async def build_data_new_node(db: InfrahubDatabase, mapping: dict[str, ConversionFieldInput], node: Node) -> dict:
-    """Value of a given field on the target kind to convert is either an input source attribute/relationship of the source node,
-    or a raw value."""
+async def build_data_new_node(
+    db: InfrahubDatabase, mapping: dict[str, ConversionFieldInput], node: Node
+) -> dict[str, Any]:
+    """Build creation payload for the new node.
+
+    Value for each target field is sourced from the input node (source_field),
+    provided as explicit data, or left to default when requested.
+
+    Args:
+        db: Database handle.
+        mapping: Destination field mapping configuration.
+        node: Source node being converted.
+
+    Returns:
+        A dict suitable for create_node with attribute values and relationship references.
+    """

98-104: Add docstring to _has_pass_thru_aware_attributes

Helps clarify agnostic conversion constraints.

Apply:

-def _has_pass_thru_aware_attributes(node_schema: NodeSchema, mapping: dict[str, ConversionFieldInput]) -> bool:
+def _has_pass_thru_aware_attributes(node_schema: NodeSchema, mapping: dict[str, ConversionFieldInput]) -> bool:
+    """Detect if any aware attributes are passed through from source to target.
+
+    Args:
+        node_schema: Schema of the source node.
+        mapping: Field mapping used for conversion.
+
+    Returns:
+        True if at least one aware attribute is passed through.
+    """

106-121: Add docstring to validate_conversion

Documenting relationship validation improves maintainability.

Apply:

-async def validate_conversion(
+async def validate_conversion(
     deleted_node: Node, branch: Branch, db: InfrahubDatabase, timestamp_before_conversion: Timestamp
 ) -> None:
+    """Validate that conversion didn't break relationship constraints.
+
+    Checks both outbound and inbound unidirectional relationships using a pre-conversion timestamp.
+
+    Args:
+        deleted_node: The original node, removed during conversion.
+        branch: Branch context.
+        db: Database handle.
+        timestamp_before_conversion: Snapshot time before the conversion started.
+    """

123-144: Add docstring to convert_and_validate_object_type

Clarify behavior and side effects (message bus refresh).

Apply:

-async def convert_and_validate_object_type(
+async def convert_and_validate_object_type(
     node: Node,
     target_schema: NodeSchema,
     mapping: dict[str, ConversionFieldInput],
     branch: Branch,
     db: InfrahubDatabase,
 ) -> Node:
+    """Convert a node and validate relationships within a transaction.
+
+    Performs conversion, validates post-conditions, and refreshes branch registry.
+
+    Args:
+        node: Source node.
+        target_schema: Target node schema.
+        mapping: Field mapping for conversion.
+        branch: Branch to apply changes to.
+        db: Database handle.
+
+    Returns:
+        The newly created node after conversion.
+    """
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1efa4e6 and f2f657a.

📒 Files selected for processing (8)
  • backend/infrahub/core/convert_object_type/object_conversion.py (7 hunks)
  • backend/infrahub/core/convert_object_type/repository_conversion.py (2 hunks)
  • backend/infrahub/graphql/mutations/convert_object_type.py (3 hunks)
  • backend/tests/conftest.py (1 hunks)
  • backend/tests/functional/convert_object_type/test_convert_object_type.py (4 hunks)
  • backend/tests/functional/convert_object_type/test_convert_repositories.py (4 hunks)
  • backend/tests/unit/core/convert_object_type/test_convert_object_type.py (9 hunks)
  • python_sdk (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • backend/infrahub/core/convert_object_type/repository_conversion.py
  • python_sdk
  • backend/infrahub/graphql/mutations/convert_object_type.py
🧰 Additional context used
📓 Path-based instructions (3)
backend/tests/**/*

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Place backend tests in backend/tests/

Files:

  • backend/tests/conftest.py
  • backend/tests/functional/convert_object_type/test_convert_repositories.py
  • backend/tests/unit/core/convert_object_type/test_convert_object_type.py
  • backend/tests/functional/convert_object_type/test_convert_object_type.py
backend/**/*

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Run backend tests with pytest or via invoke tasks

Files:

  • backend/tests/conftest.py
  • backend/tests/functional/convert_object_type/test_convert_repositories.py
  • backend/tests/unit/core/convert_object_type/test_convert_object_type.py
  • backend/infrahub/core/convert_object_type/object_conversion.py
  • backend/tests/functional/convert_object_type/test_convert_object_type.py
**/*.py

📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)

**/*.py: Use triple double quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include a brief one-line description at the top of each docstring
Add a detailed description section when additional context is needed
Document function/method parameters under an Args/Parameters section without typing information
Include a Returns section describing the return value
Include a Raises section listing possible exceptions
Provide an Examples section demonstrating usage when helpful

**/*.py: Use type hints for all function parameters and return values in Python code
Prefer asynchronous implementations when feasible (use Async whenever possible)
Define asynchronous functions with async def
Use await for asynchronous calls
Use Pydantic models instead of standard library dataclasses
All Python functions must include a docstring
Use Ruff and MyPy for linting and type checking

Use ruff and mypy to validate and lint Python files

**/*.py: Use type hints for all Python function parameters and return values
Use async/await whenever possible in Python code
Use async def for asynchronous functions
Use await for asynchronous calls within async functions
Use Pydantic models instead of dataclasses for data modeling
All Python functions must have docstrings
Docstrings must use triple quotes (""")
Follow Google-style docstring format
Docstrings should include, when applicable: brief description, detailed description, Args/Parameters (without types), Returns, Raises, and Examples
Validate and lint Python with ruff and mypy
Format Python code using the project’s formatter (invoke format)

Files:

  • backend/tests/conftest.py
  • backend/tests/functional/convert_object_type/test_convert_repositories.py
  • backend/tests/unit/core/convert_object_type/test_convert_object_type.py
  • backend/infrahub/core/convert_object_type/object_conversion.py
  • backend/tests/functional/convert_object_type/test_convert_object_type.py
🧬 Code graph analysis (3)
backend/tests/functional/convert_object_type/test_convert_repositories.py (1)
backend/infrahub/core/convert_object_type/object_conversion.py (1)
  • convert_object_type (146-201)
backend/tests/unit/core/convert_object_type/test_convert_object_type.py (1)
backend/infrahub/core/convert_object_type/schema_mapping.py (1)
  • SchemaMappingValue (7-10)
backend/tests/functional/convert_object_type/test_convert_object_type.py (1)
backend/infrahub/core/convert_object_type/object_conversion.py (1)
  • convert_object_type (146-201)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
  • GitHub Check: E2E-testing-version-upgrade / From 1.3.0
  • GitHub Check: backend-benchmark
  • GitHub Check: E2E-testing-playwright
  • GitHub Check: backend-docker-integration
  • GitHub Check: validate-generated-documentation
  • GitHub Check: backend-tests-functional
  • GitHub Check: backend-tests-unit
  • GitHub Check: backend-tests-integration
  • GitHub Check: Cloudflare Pages
🔇 Additional comments (13)
backend/tests/conftest.py (1)

1256-1259: Good addition: default favorite_color for PersonGeneric

This ensures tests using use_default_value=True resolve to "blue" deterministically.

backend/tests/functional/convert_object_type/test_convert_object_type.py (4)

6-6: Consistent use of SDK models

Importing ConversionFieldInput/Value from the SDK aligns with the PR goal to avoid duplication.


62-66: Mapping includes favorite_color as expected

Schema mapping correctly exposes favorite_color for conversion.


116-121: Explicit source data includes favorite_color

Setting favorite_color on the source plus mapping use_default_value=True effectively tests override by default.


153-153: Asserting default favorite_color is correct

Confirms default application when use_default_value=True.

backend/tests/functional/convert_object_type/test_convert_repositories.py (3)

213-218: Mapping reconstruction looks correct

Wrapping attribute_value with ConversionFieldValue and dumping to JSON for GraphQL input is appropriate.


359-364: Good: source_field mappings for read-only -> read-write conversion

This aligns with the computed schema mapping.


498-503: Good: provide default_branch via attribute_value

Correctly supplies the required input for conversion.

backend/tests/unit/core/convert_object_type/test_convert_object_type.py (4)

39-39: Updated mapping count reflects added favorite_color

Assertion is correct.


44-46: Mapping includes favorite_color as an attribute

Matches schema expectations.


110-118: Use of ConversionFieldInput/Value is correct

Covers attributes, ONE/MANY relationships, and explicit peers.


141-141: Asserting default favorite_color "blue" is appropriate

Confirms default application without explicit mapping.

backend/infrahub/core/convert_object_type/object_conversion.py (1)

23-30: Honor explicit nulls in ConversionFieldValue

attribute_value=None should be allowed to clear optional attributes. Current checks treat None as unset and wrongly raise.

Apply:

-def _get_conversion_field_raw_value(conv_field_value: ConversionFieldValue) -> Any:
-    if conv_field_value.attribute_value is not None:
-        return conv_field_value.attribute_value
-    if conv_field_value.peer_id is not None:
-        return conv_field_value.peer_id
-    if conv_field_value.peers_ids is not None:
-        return conv_field_value.peers_ids
-    raise ValueError("ConversionFieldValue has not been validated correctly.")
+def _get_conversion_field_raw_value(conv_field_value: ConversionFieldValue) -> Any:
+    # Distinguish unset vs explicitly set to None
+    raw = conv_field_value.model_dump(exclude_unset=True)
+    if "attribute_value" in raw:
+        return raw["attribute_value"]
+    if "peer_id" in raw:
+        return raw["peer_id"]
+    if "peers_ids" in raw:
+        return raw["peers_ids"]
+    raise ValueError("ConversionFieldValue has not been validated correctly.")

@LucasG0 LucasG0 force-pushed the lgu-obj-conv-default-value branch from f2f657a to 819e0c0 Compare September 25, 2025 12:57
@LucasG0 LucasG0 force-pushed the lgu-obj-conv-default-value branch from 819e0c0 to 55f404f Compare September 25, 2025 12:57
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f2f657a and 55f404f.

📒 Files selected for processing (8)
  • backend/infrahub/core/convert_object_type/object_conversion.py (7 hunks)
  • backend/infrahub/core/convert_object_type/repository_conversion.py (2 hunks)
  • backend/infrahub/graphql/mutations/convert_object_type.py (3 hunks)
  • backend/tests/conftest.py (1 hunks)
  • backend/tests/functional/convert_object_type/test_convert_object_type.py (4 hunks)
  • backend/tests/functional/convert_object_type/test_convert_repositories.py (4 hunks)
  • backend/tests/unit/core/convert_object_type/test_convert_object_type.py (9 hunks)
  • python_sdk (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
  • backend/tests/functional/convert_object_type/test_convert_repositories.py
  • backend/tests/conftest.py
  • backend/tests/functional/convert_object_type/test_convert_object_type.py
  • python_sdk
  • backend/infrahub/graphql/mutations/convert_object_type.py
🧰 Additional context used
📓 Path-based instructions (3)
backend/**/*

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Run backend tests with pytest or via invoke tasks

Files:

  • backend/infrahub/core/convert_object_type/repository_conversion.py
  • backend/infrahub/core/convert_object_type/object_conversion.py
  • backend/tests/unit/core/convert_object_type/test_convert_object_type.py
**/*.py

📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)

**/*.py: Use triple double quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include a brief one-line description at the top of each docstring
Add a detailed description section when additional context is needed
Document function/method parameters under an Args/Parameters section without typing information
Include a Returns section describing the return value
Include a Raises section listing possible exceptions
Provide an Examples section demonstrating usage when helpful

**/*.py: Use type hints for all function parameters and return values in Python code
Prefer asynchronous implementations when feasible (use Async whenever possible)
Define asynchronous functions with async def
Use await for asynchronous calls
Use Pydantic models instead of standard library dataclasses
All Python functions must include a docstring
Use Ruff and MyPy for linting and type checking

Use ruff and mypy to validate and lint Python files

**/*.py: Use type hints for all Python function parameters and return values
Use async/await whenever possible in Python code
Use async def for asynchronous functions
Use await for asynchronous calls within async functions
Use Pydantic models instead of dataclasses for data modeling
All Python functions must have docstrings
Docstrings must use triple quotes (""")
Follow Google-style docstring format
Docstrings should include, when applicable: brief description, detailed description, Args/Parameters (without types), Returns, Raises, and Examples
Validate and lint Python with ruff and mypy
Format Python code using the project’s formatter (invoke format)

Files:

  • backend/infrahub/core/convert_object_type/repository_conversion.py
  • backend/infrahub/core/convert_object_type/object_conversion.py
  • backend/tests/unit/core/convert_object_type/test_convert_object_type.py
backend/tests/**/*

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Place backend tests in backend/tests/

Files:

  • backend/tests/unit/core/convert_object_type/test_convert_object_type.py
🧬 Code graph analysis (1)
backend/tests/unit/core/convert_object_type/test_convert_object_type.py (1)
backend/infrahub/core/convert_object_type/schema_mapping.py (1)
  • SchemaMappingValue (7-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: backend-tests-functional
  • GitHub Check: backend-docker-integration
  • GitHub Check: backend-tests-unit
  • GitHub Check: backend-tests-integration
🔇 Additional comments (3)
backend/infrahub/core/convert_object_type/repository_conversion.py (1)

4-7: SDK type swap looks good.
Importing ConversionFieldInput directly from the shared SDK keeps the repository conversion flow aligned with the new shared models.

backend/tests/unit/core/convert_object_type/test_convert_object_type.py (1)

138-145: Nice coverage on defaulted attributes.
Asserting that favorite_color falls back to its default guards the new use_default_value pathway from regressions.

backend/infrahub/core/convert_object_type/object_conversion.py (1)

23-30: Allow explicit null conversions for attributes.
When a caller sends ConversionFieldValue(attribute_value=None) to clear an optional field, this helper falls through all branches and raises ValueError. That regresses explicit-null handling and prevents users from blanking a field during conversion. Please switch to checking which fields were actually set (e.g., via model_dump(exclude_unset=True)) so that explicitly provided None values are propagated.

Apply this diff:

-def _get_conversion_field_raw_value(conv_field_value: ConversionFieldValue) -> Any:
-    if conv_field_value.attribute_value is not None:
-        return conv_field_value.attribute_value
-    if conv_field_value.peer_id is not None:
-        return conv_field_value.peer_id
-    if conv_field_value.peers_ids is not None:
-        return conv_field_value.peers_ids
+def _get_conversion_field_raw_value(conv_field_value: ConversionFieldValue) -> Any:
+    raw_value = conv_field_value.model_dump(exclude_unset=True)
+    if "attribute_value" in raw_value:
+        return raw_value["attribute_value"]
+    if "peer_id" in raw_value:
+        return raw_value["peer_id"]
+    if "peers_ids" in raw_value:
+        return raw_value["peers_ids"]
     raise ValueError("ConversionFieldValue has not been validated correctly.")

Comment on lines +23 to +30
def _get_conversion_field_raw_value(conv_field_value: ConversionFieldValue) -> Any:
if conv_field_value.attribute_value is not None:
return conv_field_value.attribute_value
if conv_field_value.peer_id is not None:
return conv_field_value.peer_id
if conv_field_value.peers_ids is not None:
return conv_field_value.peers_ids
raise ValueError("ConversionFieldValue has not been validated correctly.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Add docstring for _get_conversion_field_raw_value.
New helpers must ship with Google-style docstrings per our Python guidelines.

Apply this diff:

-def _get_conversion_field_raw_value(conv_field_value: ConversionFieldValue) -> Any:
+def _get_conversion_field_raw_value(conv_field_value: ConversionFieldValue) -> Any:
+    """Extract the raw payload carried by a ConversionFieldValue.
+
+    Args:
+        conv_field_value: Conversion data for the destination field.
+
+    Returns:
+        The attribute value, peer identifier, or list of peer identifiers.
+
+    Raises:
+        ValueError: If the conversion input does not define any payload.
+    """
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def _get_conversion_field_raw_value(conv_field_value: ConversionFieldValue) -> Any:
if conv_field_value.attribute_value is not None:
return conv_field_value.attribute_value
if conv_field_value.peer_id is not None:
return conv_field_value.peer_id
if conv_field_value.peers_ids is not None:
return conv_field_value.peers_ids
raise ValueError("ConversionFieldValue has not been validated correctly.")
def _get_conversion_field_raw_value(conv_field_value: ConversionFieldValue) -> Any:
"""Extract the raw payload carried by a ConversionFieldValue.
Args:
conv_field_value: Conversion data for the destination field.
Returns:
The attribute value, peer identifier, or list of peer identifiers.
Raises:
ValueError: If the conversion input does not define any payload.
"""
if conv_field_value.attribute_value is not None:
return conv_field_value.attribute_value
if conv_field_value.peer_id is not None:
return conv_field_value.peer_id
if conv_field_value.peers_ids is not None:
return conv_field_value.peers_ids
raise ValueError("ConversionFieldValue has not been validated correctly.")
🤖 Prompt for AI Agents
In backend/infrahub/core/convert_object_type/object_conversion.py around lines
23 to 30, the helper function _get_conversion_field_raw_value lacks a
Google-style docstring; add a docstring immediately below the def that documents
the function purpose, the conv_field_value parameter (type
ConversionFieldValue), the return value (Any) describing possible returned
fields (attribute_value, peer_id, peers_ids), and the ValueError raised when
none are set, following our Google-style format (Args, Returns, Raises) and
keeping it concise.

@LucasG0 LucasG0 merged commit 7a904a7 into develop Sep 25, 2025
34 checks passed
@LucasG0 LucasG0 deleted the lgu-obj-conv-default-value branch September 25, 2025 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

group/backend Issue related to the backend (API Server, Git Agent)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants